Conversation
|
i started on this too, im working on the nock testing bit as undici fetch uses mocks instead socks proxy was a rabbithole too i havent found a solution for that yet |
|
@mstruebing @davidgamero I tried to give it a go, however many apis use "fetch" directly, instead of using the same "configuration" pattern in other regular APIs. Would you prefer if the APIs are to be rewritten to match the same pattern? in the spirit of my previous PR updating watch_api? I think if all APIs are aligned, it would be easier to update to undici, but it will most definitely be a breaking change |
|
these apis currently use
|
|
@copilot please fix this PR |
|
@brendandburns I've opened a new pull request, #2794, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: brendandburns <5751682+brendandburns@users.noreply.github.com>
Co-authored-by: brendandburns <5751682+brendandburns@users.noreply.github.com>
…gent Co-authored-by: brendandburns <5751682+brendandburns@users.noreply.github.com>
fix: migrate API client HTTP transport from node-fetch to undici
|
I tried a copilot fix for this which is now applied, it modifies a bunch of the tests, I did some manual verification, but would love a second pair of eyes. |
|
@copilot please fix tests, looks like File is not defined? Maybe this is a nodejs 18 thing? |
|
@brendandburns I've opened a new pull request, #2797, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: brendandburns <5751682+brendandburns@users.noreply.github.com>
…gain [WIP] [#2738] Regenerate OpenAPI generator
|
@copilot please fix the remaining 3 failing tests. |
src/config_test.ts
Outdated
| strictEqual( | ||
| (dispatcher as any)[ | ||
| Object.getOwnPropertySymbols(dispatcher!).find((s) => s.toString() === 'Symbol(options)')! | ||
| ].connect.servername, |
There was a problem hiding this comment.
i echo the previous comments on private state extraction of symbols @cjihrig
as many of these tests are essentially validating args to the UndiciAgent | UndiciProxyAgent constructors,
proposal to extract a createDispatcherOptions method from createDispatcher that shares a parameter signature ex:
private createDispatcherOptions(
cluster: Cluster | null,
agentOptions: https.AgentOptions
): UndiciAgentOptions | UndiciProxyAgentOptions we can test that method directly, and the createDispatcher function serves to pass those options to constructors exclusively.
this would remove the need for much of the fragile introspection
|
@copilot try out the idea to extract a createDispatcherOptions method in a PR and note if it can effectively reduce reliance on symbol internals by testing the options generation instead of the objects post-constructor invocation:
private createDispatcherOptions(
cluster: Cluster | null,
agentOptions: https.AgentOptions
): UndiciAgentOptions | UndiciProxyAgentOptions
|
…ne-more-time refactor: replace try/finally with t.after() hooks in tests
|
made a PR into this branch for the options extraction here #2801 |
…her-testing extract dispatcher options construction method
|
@copilot please fix tests again, looks like the undici version got moved to version 7 again and it should be version 6 |
|
@brendandburns I've opened a new pull request, #2802, to work on those changes. Once the pull request is ready, I'll request review from you. |
…dici path Co-authored-by: brendandburns <5751682+brendandburns@users.noreply.github.com> Agent-Logs-Url: https://github.com/kubernetes-client/javascript/sessions/f6da75b3-9f5b-4cfa-9fbe-0191093345f3
…ient connector Co-authored-by: brendandburns <5751682+brendandburns@users.noreply.github.com> Agent-Logs-Url: https://github.com/kubernetes-client/javascript/sessions/789ac2b6-243f-4ac2-9786-5e5e23942604
…lease-work fix: downgrade undici to v6 and add SOCKS proxy support for undici dispatcher path
|
small format fix that's blocking the tests #2806 |
style: format src/config.ts with prettier
|
Personally, I think this is ready to merge, anyone else want to take another look? |
cjihrig
left a comment
There was a problem hiding this comment.
Agreed this looks good enough. I think anything else can be addressed in a follow up.
/lgtm
/approve
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cjihrig, mstruebing The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This regenerates the with the new openapi generator after the changes from #2738
The code needs to be adjusted the generator now works wit undici.
Help would be appreciated, I've tried it two times but always went into deep rabbit holes.